Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add basic latency benchmarks for migrations #2665

Merged
merged 2 commits into from
May 25, 2021

Conversation

piotr-iohk
Copy link
Contributor

@piotr-iohk piotr-iohk commented May 24, 2021

Issue Number

ADP-680

Overview

  • 2c31861
    Add basic latency benchmarks for migrations

Comments

Migration Plan
Migration

@piotr-iohk piotr-iohk self-assigned this May 24, 2021
Copy link
Member

@jonathanknowles jonathanknowles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍🏻

I only have one suggestion about the name of the wallet.

{ passphrase: #{fixturePassphrase}
, addresses: #{addresses}
}|]
fmtResult "postMigration " t12b
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

@@ -203,15 +204,16 @@ walletApiBench capture ctx = do
nFixtureWallet n = do
wal1 : wal2 : _ <- replicateM n (fixtureWallet ctx)
walMA <- fixtureMultiAssetWallet ctx
pure (wal1, wal2, walMA)
walMAForMigration <- fixtureMultiAssetWallet ctx
pure (wal1, wal2, walMA, walMAForMigration)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @piotr-iohk! This is just a suggestion, but perhaps we could use a name that makes it obvious that it's a source wallet to be migrated:

Suggested change
pure (wal1, wal2, walMA, walMAForMigration)
pure (wal1, wal2, walMA, maWalletToMigrate)

What do you think about this name?

("For migration" could imply that it's a target wallet.)

@piotr-iohk
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request May 25, 2021
2665: Add basic latency benchmarks for migrations r=piotr-iohk a=piotr-iohk

# Issue Number

ADP-680


# Overview

- 2c31861
  Add basic latency benchmarks for migrations


# Comments

[Migration Plan](http://cardano-wallet-benchmarks.herokuapp.com/latency?latency_category=4&latency_benchmark=all&latency_measurement=postMigrationPlan)
[Migration](http://cardano-wallet-benchmarks.herokuapp.com/latency?latency_category=4&latency_benchmark=all&latency_measurement=postMigration)


Co-authored-by: Piotr Stachyra <piotr.stachyra@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 25, 2021

Build failed:

#timeout

@piotr-iohk
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request May 25, 2021
2665: Add basic latency benchmarks for migrations r=piotr-iohk a=piotr-iohk

# Issue Number

ADP-680


# Overview

- 2c31861
  Add basic latency benchmarks for migrations


# Comments

[Migration Plan](http://cardano-wallet-benchmarks.herokuapp.com/latency?latency_category=4&latency_benchmark=all&latency_measurement=postMigrationPlan)
[Migration](http://cardano-wallet-benchmarks.herokuapp.com/latency?latency_category=4&latency_benchmark=all&latency_measurement=postMigration)


Co-authored-by: Piotr Stachyra <piotr.stachyra@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 25, 2021

Build failed:

#timeout

@piotr-iohk
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request May 25, 2021
2665: Add basic latency benchmarks for migrations r=piotr-iohk a=piotr-iohk

# Issue Number

ADP-680


# Overview

- 2c31861
  Add basic latency benchmarks for migrations


# Comments

[Migration Plan](http://cardano-wallet-benchmarks.herokuapp.com/latency?latency_category=4&latency_benchmark=all&latency_measurement=postMigrationPlan)
[Migration](http://cardano-wallet-benchmarks.herokuapp.com/latency?latency_category=4&latency_benchmark=all&latency_measurement=postMigration)


2667: Add property test for `UTxOIndex.selectRandomWithPriority`. r=jonathanknowles a=jonathanknowles

# Issue Number

ADP-890

# Overview

This PR adds a property test for `UTxOIndex.selectRandomWithPriority`.

The `selectRandomWithPriority`  function is designed to:
- select an entry at random from a UTxO index according to a specified list of filter conditions;
- traverse the specified list of filter conditions in order of priority **_from left to right_**.

The test added in this PR provides a basic sanity check to verify that priority order is respected.

# Sample Output

```hs
Cardano.Wallet.Primitive.Types.UTxOIndex
  Indexed UTxO set properties
    Index Selection
      prop_selectRandomWithPriority
        +++ OK, passed 1600 tests:
        59.69% have match for neither asset 1 nor asset 2
        17.12% have match for asset 1 but not for asset 2
        16.31% have match for asset 2 but not for asset 1
         6.88% have match for both asset 1 and asset 2

Finished in 1.0870 seconds
1 example, 0 failures
```

# QA Due Diligence

I ran this test 500 times to increase confidence that it will not fail spuriously. No failures were encountered.


Co-authored-by: Piotr Stachyra <piotr.stachyra@iohk.io>
Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 25, 2021

Build failed (retrying...):

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 25, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit e054c8b into master May 25, 2021
@iohk-bors iohk-bors bot deleted the piotr/latency-benchmark-for-migration branch May 25, 2021 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants